Extract the resolving of a page's path and test it #232
Conversation
* Extracts the resolving of a page's path to it's own file for testability. * Adds `ava` as a development dependency to write and run tests. * Adds ava related settings to inherit the babel config and run the tests with npm test to package.json * Refactor the quad-nested if statements in path resolving. Working toward resolving Issue gatsbyjs#154
|
Wow! This is fantastic. A great refactor + tests! Yeah I don't really like classes generally. The approach you came up with is very readable and should be easy to maintain/extend going forward. I don't have strong feelings about tests structure so what you've setup sounds great. @gatsbyjs/gatsby-core-maintainers any thoughts? |
|
+1 for using AVA as test runner. Although be aware that AVA will probably break on old node (<5) if there are too many files |
* Split up all the spaghetti in glob-pages up so that it can be unit tested. * Renamed `pathresolver` to `urlResolver` * Added tests to verify the structure of the data returned by the new pathResolver. * updated the urlResolver's tests with the new function name
|
I had some time tonight and split up glob pages to the point where I think it can be unit tested effectively. Do you guys see any problems with the way that I split it up? Would you have split it up differently? Renamed the On a side note I just noticed while looking for some documentation that ava is changing the ok/notok to falsey/truthy in the very near future, so some minor refactoring will be required. |
|
@benstepp really awesome stuff! I didn't have time to get it a full review but things looked good on my first path through. I'll give this a through review this Saturday (along with the other open PRs). And 100% agree |
|
I love that we're bringing tests into the project! I have felt the pain of making changes in gatsby without them. However, unit tests are always a bit scary by themselves, so we now have a false sense of security about the whole project if Would you consider adding a basic overall test scenario? Maybe just a couple files, run gatsby, validate that it runs successfully, puts files on disk? |
|
@scottnonnenberg I totally agree e2e tests are needed. I have a number of ideas toward that end in #154. If @benstepp (or you) want to tackle that hallelujah but I think @benstepp meant this PR to be an incremental step along the way to full test coverage :-) |
|
#226 would be a good place to start. Adding them as submodules / test-fixtures could integrate them into the test suite. This does add a lot of developer overhead though, as the dependency count is pretty big, and we would have to keep them up to date. Not sure if you would want to do a full integration test with actually calling the cli in shell, or abstract the cli from gatsby so that all it does is call a function to Also I don't think ava has test-tagging yet. With RSpec (ruby land, super strong) you could tag a test as slow and then skip running them, but I'm sure we could come up with something. Maybe a naming convention with test files and then two package.json scripts to target specifically named files and run And yes, the PR is meant as an incremental step. Writing the first tests for a project are always hard, and you would usually test from the outside-in (integration into unit tests), but I felt like that would require too much reorganization to pull off quickly. |
|
@benstepp fantastic work! This is a really great start to getting Gatsby's testing story going! Thanks for all your contributions so far. I added you as a contributor to the repo btw :-) |
|
I did need to make one minor fix to your refactoring as metadata from pages weren't being used in finding the path or returned correctly. 0ce3be5 |
|
I also added a eslint plugin for linting Ava which seems pretty cool 5ab7a0c https://github.com/sindresorhus/eslint-plugin-ava |
|
Hiya @benstepp! 👋 This is definitely late, but on behalf of the entire Gatsby community, I wanted to say thank you for being here. Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If you have questions, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! 💪💜 |
testability.
avaas a development dependency to write and run tests.tests with npm test to package.json
Working toward resolving Issue #154 one step at a time.
Not too sure on code style, so I didn't want to start adding es6 classes when there were none in the project. I opted for a closure with named functions to provide some readability instead of a super-if statement. The named functions also open up a nice readable place to fix user defined paths in frontmatter (#223) and allow for absolute/relative paths defined in frontmatter(#221).
Also not too sure if the gatsby team has any testing preferences on how to organize tests / readable descriptions for tests. I just matched the lib file structure in the test directory. First time using ava (well second. we tried it on an internal project at work and it started to choke around 100+ test files, but the ava team is working on fixing that).
Let me know if you have any feedback/review.